Please include constraints, explained in plain sentences, in the GoDoc.

I'll update this.

While I know that this is a machine API and as such the validations run through a webhook, we no longer recommend the use of the kubebuilder:validation:Pattern marker and prefer using the kubebuilder:validation:XValidation marker to write CEL expressions with more user-friendly error messages than what would be returned by the pattern marker.

sure, i can make this change

What is the difference between setting this to "" and omitting the field entirely?

There should be no difference. I would assume this field is not set if user not intending to place instances into a dedicated host.

If there is no difference, this should not be a pointer and should have a minimum length of 1. This is probably what the linter is complaining about.

Since this is validated by Go based webhooks, and not openapi, the linter is wrong on this one.

If we make this not a pointer, then the Go code has no way to know if this was deliberately set to "" or not. We don't want "" to be valid, so this needs to be a pointer so that we can check that.

In this case (and future cases like this in these providerspec APIs) we will want to make exceptions to the serialization rules on the linter.

We may want to even disable the serialization rules on these particular APIs somehow 🤔

Ah, I went into standard API review mode here and forgot this API is webhook validation 🤦

Thanks for catching that!

We may want to even disable the serialization rules on these particular APIs somehow

Can we do this via codegen configurations?

Can we do this via codegen configurations?

No, but we should be able to disable using the .golangci-lint.yaml config, ideally we could have a different config for the APIs that act like this, these MAPI ones aren't the only ones (e.g. the aggregated APIs we support too)

Whether it is set to host or default, hostID will be required?

Also, when referring to a field use the serialized form of the field name as that is what end-users would be familiar with:

Yes, if hostID is set, this field will be could required. The idea was that if they do not set this field when hostID is set, it will default to host behavior.

To clarify, hostID is required when hostAffinity is set to host and should be forbidden otherwise?

Correct. I'll make sure this godoc states this. I have the webhook logic doing this already.

What happens if I don't supply this value?

It will default to host. I have been playing with trying to make linter happy with the configs today (which I put this into WIP). I will update the godoc to reflect this default behavior shortly.

So are the options here actually omitted (i.e don't set hostAffinity and hostID to use default AWS affinity logic) and Host (i.e I want it to use be assigned to this host)?

So I am discussing this with @rvanderp3 , "Default" we are gonna change to "AnyAvailable". I believe with the AWS API, the idea is this field dictates whether hostID is looked at or not. We are gonna confirm this and see what the behavior should be to clear up any confusions.

For enum fields we generally try to follow the pattern of:

hostAffinity ...
Allowed values are Host, Default, and omitted.
When set to Host, ...
When set to Default, ...
When omitted, ...

Ok, i'll make this godoc update

Enums should be CamelCase:

I thought it should. I was copying what was in CAPA. I'll make these adjustments.

How is setting "" different than omitting the field that uses this type altogether?

This was an attempt to make linter happy. I'll see if I can drop this which is what I am attempting.

While this regex is pretty straightforward, we prefer to return a message that is a plain english description on a validation error here. Makes it more human readable.

I attempted to make this better. hopefully it makes more sense in non regex speak.

We should probably add a min length here as well so that an empty string ("") isn't a valid value. Linter will complain about this because it isn't aware of the machine API nuance, but we can override that.

I can add that. Thanks!

Can we specify what "hexadecimal characters" are succinctly? Maybe something like:

I'll update this

What does it mean for this field to be omitted? We should make this clear for end-users.

I think I shortened two sentences and left this in. I'll remove since it is marked optional

The more I look at this, the more I wonder if this should be nested another level and follow the discriminated union pattern.

It might make it easier to have configuration options be like:

...
dedicatedHost:
  affinity: Host
  host:
    id: h-017afcd

and

...
dedicatedHost:
  affinity: AnyAvailable

Then we can enforce requirements like dedicateHost.host.id being required when setting dedicatedHost.affinity to Host and forbidding it otherwise.

Sure, I discussed with @rvanderp3 and we can make it this way.

Max length isn't needed on enums, but the type alias would need the enum marker.

I had to add that to make linter happy.

make: Leaving directory '/home/ngirard/GitProjects/api-ngirard/tools'
machine/v1beta1/types_awsprovider.go:130:2: maxlength: field HostAffinity type HostAffinity must have a maximum length, add kubebuilder:validation:MaxLength marker (kubeapilinter)
        HostAffinity *HostAffinity `json:"hostAffinity,omitempty"`
        ^
1 issues:
* kubeapilinter: 1

That is likely because you don't have the +kubebuilder:validation:Enum marker on the type that it is asking for the max length marker. The linter can also pick up usage of the enum marker to determine that the max length isn't necessary (inherently enforced by the longest allowed value)

I don't think we need the self == null in the CEL expression. IIRC it just won't run the validation if the field is omitted, which it would be on a nil value (even though this goes through a webhook, having a correct expression here helps with future readers to understand the intent without confusion).

ok, i can make that update.

The "discriminant" should be required so an end user has to make an explicit decision here - hostAffinity is the discriminant here. Because the parent field is already optional it is safe to make this field required.

I wonder if affinity should be outside the dedicated host section. This way, we use the DedicatedHost field and have ID under it.

...
hostAffinity: Host
dedicatedHost:
    id: h-017afcd

Discriminants are enums and do not need a MaxLength. On the HostAffinity Go type alias, add +kubebuilder:validation:Enum=AnyAvailable;Host and remove the +kubebuilder:validation:MaxLength marker from this set of markers

I can, but i was hitting error on this before where it was requiring it. I"ll try again and see how it goes.

Because this is now nested under a parent field called dedicatedHost we can probably just call this field affinity instead of hostAffinity. Calling it hostAffinity results in a "stutter" like dedicatedHost.hostAffinity

ok

We have a general pattern we try to follow for consistency across OpenShift APIs.

Could we update this to look like this?:

hostAffinity selects how the instance is placed on a dedicate host.
Allowed values are AnyAvailable and Host.
When set to AnyAvailable, the platform selects any available dedicated host.
When set to Host, the instance must run on a specific host denoted by host.hostID.
host is required when hostAffinity is set to Host, and forbidden otherwise.

sure, i can remove formatting and use sentences.

I'm wondering if Host makes the most sense here or if it is to stutter-y.

As an example:

dedicatedHost:
  hostAffinity: Host
  host:
    hostID: h-1326af

That has "host" 5 times in 4 lines.

Maybe something like:

dedicatedHost:
  affinity: Specific
  specific:
    id: h-1326af

is better here?

well above we said remove host from affinity, but i do not like the "specific" being used. That feels very odd. So this goes back to not doing discriminating union due to how this is now looking more complex. With this, affinity states if it is to be assigned to a dedicated host and then we just need a field to specify the host ID. Currently we do not have other info for the host to provide so it may start to feel like overkill.

Maybe taking a step back here would be helpful.

Maybe I misunderstood, but my interpretation was that the original hostAffinity value was meant to only be used with dedicated hosts. Is that incorrect?

No, you understand correctly. The complexity is coming from the discriminating union additions, but I just want to find verbage that makes sense and is clear. So allow me to explain history and ideas.

In the aws API (https://docs.aws.amazon.com/AWSEC2/latest/UserGuide/moving-instances-dedicated-hosts.html) it states the placement logic for an instance. Its confusing there too. Here is an example:

aws ec2 modify-instance-placement \
    --instance-id i-1234567890abcdef0 \
    --affinity host \
    --tenancy host \
    --host-id h-012a3456b7890cdef

So what we are adding / added in CAPA upstream was this: kubernetes-sigs/cluster-api-provider-aws#5548

So for MAPI, we are adding just enough so we can do the mapi2capi conversion logic so we can create these resources. Ideally I was hoping to keep our API similar for simplicity (the original concept at beginning of PR), but I am happy to have it follow the OCP ideals (I did similar things for CAPV stuff for multi disk and others). So with all of this, I do like the idea of using the discriminating union pattern. Building upon our ideas with thoughts on whats next to come might be to do a combination of our ideas with the following:

[Any Host Placement]

hostPlacement:
  hostAffinity: AnyAvailable

[Static Placement]

hostPlacement:
  hostAffinity: DedicatedHost
  DedicatedHost:
    id: h-abcdef0123456789a

[Dynamic Host Placement]

hostPlacement:
  hostAffinity: DynamicHost
  DynamicHost:
    tags:
    - app: myApp
    - department: whatever

Maybe this is more in line with what you are thinking. I know @rvanderp3 is working on adding dynamic host support upstream at the moment and that will require some additional data or changed fields. Maybe this better sets us up for that.

What are your thoughts?

I think we want something more like:

host specifies the exact host that an instance should run on.
host is required when hostAffinity is set to Host, and forbidden otherwise.

i'll make the change.

I think we can remove affinity text here. We should already be gating usage of the parent field based on what the affinity value is set to.

sounds good

With this new structure, I believe this should be a required field

I can update this.